fix(orchestrator): give the dispatch lease back when a release is dead-lettered - #391

DO NOT MERGE without the principal's gate. Live P0 investigation; opened for review.

What production was doing

0.1.79 (which already contains #379) dispatched nothing for three days. Four issues — including a canary filed purely to test dispatch — were all simultaneously blocked on the same shape:

[factory] durable dispatch is leased by another publisher; waiting for lease release
  {"issue":"1540","leaseRemainingMs":189205,"retryMs":1000}
  {"issue":"2185","leaseRemainingMs":189021} {"issue":"3032","leaseRemainingMs":189112} {"issue":"390","leaseRemainingMs":189310}

while the release for 1540's agents failed, continuously, all day:

[factory] failed to release ar-1540-impl-relay during completion
  RelayError: Agent "ar-1540-impl-relay" has no live host node; cannot dispatch release
  code transport_error, statusCode 503, rawCode agent_host_unavailable, retryable true

Does the 503 path charge the release-attempt counter? Yes.

The working hypothesis going in was that a retryable: true 503 takes a different branch and re-arms without charging, making #379's bound a no-op on the case it was written for. That is not what happens, and the hypothesis is refuted.

#releaseAndTerminateAgents has exactly one classifier on the release error, isAgentAlreadyGoneOnRelease, and release-error.ts documents — and release-error.test.ts:48 already asserts — that a 503 agent_host_unavailable is not "already gone". So it falls to the else, lands in failed[], and produces the verbatim production log line. #finishDurableRelease then sees failed.length > 0 and calls #scheduleReleaseRetry, which on the durable lifecycle calls #scheduleDispatchLifecycleRetry(..., { releaseAttempt: true }), which calls #chargeReleaseAttempt. A 503 is charged exactly like the plain Error the #379 suite throws.

This is confirmed empirically, not by reading: the first two assertions of the new tests — the dispatchLifecycleReleaseAbandoned counter reaching 1, and the release retries exhausted; abandoning cleanup for this work unit error being logged — pass on unmodified origin/main when driven by the verbatim production RelayError.

A correction to the brief while we're here: #379's suite does already cover the remote/durable resolve-then-re-arm cycle, via DurableCompletionReleaseFailingFleetClient extends RemoteLifecycleFleetClient, with a test named "exhausts the budget on the durable lifecycle, where the failed release resolves instead of throwing". The gap was not the placement locality and not the resolve-vs-throw shape.

Why the lease never expires

#dispatchLifecycleEpochs is not merely a cache. #renewDispatchLifecycles walks it every DISPATCH_LIFECYCLE_RENEW_MS (60 s) and re-stamps a full DISPATCH_LIFECYCLE_LEASE_MS (300 s) onto every key it finds, unconditionally.

#releaseDeadLetteredSlot handed back the batch slot and left the key in that map. So a work unit this process had permanently sworn off driving kept a lease that was renewed for the life of the process. leaseRemainingMs of ~189 s against the 300 s TTL is precisely that renewal, observed 111 s in — the lease is not stuck, it is being actively re-stamped.

The releasing row is retained on purpose, so a successor or a restart can re-drive the cleanup with a fresh budget. But a successor first has to claim the row, and a lease renewed forever by a process that will never finish is a claim nobody can win. That is a livelock, and it is strictly worse than the spin #379 replaced: the spin was loud and process-local, whereas a retained lease on a non-terminal row is silent and blocks every other publisher, a restart of this one included.

Worth noting for the record: a retained releasing row does not consume durable batch capacity (dispatchPhaseOccupiesSlot excludes releasing), so the fleet-wide stall is not a batchSize exhaustion. The blocking is strictly per-key, which means each of the four blocked issues has its own retained lease.

The fix

Relinquish durable ownership wherever the release budget declines a re-arm.

• New #relinquishDispatchLifecycleLease(key, issueKey): drops the epoch first so a renewal tick already in flight cannot re-stamp after the release, then calls releaseDispatchLifecycleLease. If that durable call fails, dropping the epoch alone still ends the livelock — nothing renews the lease any more, so it expires within its TTL instead of never.
• #releaseDeadLetteredSlot calls it alongside handing back the batch slot, before admitting anything new and before dispatch gets a chance to throw.
• #chargeReleaseAttempt calls it on its abandoned early return as well, not only on the transition. This closes a re-entry hole: #driveDispatchLifecycle re-claims the lease at the top of every drive, before it has read the phase, so anything that drives an already-dead-lettered key — the held-agent-deadline sweep (which calls #finishDurableRelease directly at HELD_DEADLINE_OVERDUE_RETRY_MS = 1 s and consults a different abandoned set), a registry restore, a takeover — would otherwise put the epoch straight back into the renewal map and re-arm the livelock the bound just escaped.

This extends #379's dead-letter rather than adding a parallel mechanism, as asked. #379's structural test (charges the release budget from the release scheduler only, which pins #chargeReleaseAttempt to exactly two call sites) still passes unchanged.

Deliberately NOT done: classifying agent_host_unavailable as terminal

The brief asked for an argument either way. Against:

1. isAgentAlreadyGoneOnRelease returning true means the release succeeded — the agent is gone. A 503 does not establish that. The host may be briefly unreachable while the agent process is alive and holding a shared per-issue worktree. Classifying it as gone would checkpoint releasedAtMs, mark the invocation non-dispatchable, and let #cleanupAgentWorktrees run against a live worker. release-error.ts already warns about exactly this ("a 5xx from the broker itself is a real fault and must not silently succeed").
2. The retryable/terminal distinction is real, but it is a distinction in time, not in error code. A host that is briefly down and a host that is permanently gone emit an identical 503; no single response separates them. What separates them is "have we been failing on this for a while", which is what the attempt budget already measures. The budget is the terminal classifier. It simply was not wired to release ownership — which is what this PR fixes.

Tests

Two tests reproducing the production shape on the remote/durable lifecycle, throwing the verbatim RelayError (name: 'RelayError', code: 'transport_error', retryable: true, statusCode: 503, rawCode: 'agent_host_unavailable').

They assert through the invariant — a successor publisher can claim the key — rather than through a counter, so they state the property rather than the implementation, and the poll is read-only so it cannot itself take the lease it is proving is available.

Red (final tests, unmodified src/orchestrator/factory.ts)

 × a dead-lettered release must not keep the durable dispatch lease > frees the lease once the release budget is exhausted, so another publisher can claim the key 10342ms
   → expected false to be true // Object.is equality
 × a dead-lettered release must not keep the durable dispatch lease > does not let the renewal interval re-stamp the abandoned key 10181ms
   → expected false to be true // Object.is equality
      Tests  2 failed | 652 skipped (654)

Both fail by exhausting their own 10 s deadline waiting for a lease that never becomes free — the failure is a property of the loop, not of a number chosen in the test. Note that everything before that wait passes on main: the dead-letter fires, the "release retries exhausted" error is logged. That is the Step 1 answer stated as a test.

Green

 ✓ a dead-lettered release must not keep the durable dispatch lease > frees the lease once the release budget is exhausted, so another publisher can claim the key 263ms
 ✓ a dead-lettered release must not keep the durable dispatch lease > does not let the renewal interval re-stamp the abandoned key 366ms
      Tests  2 passed | 652 skipped (654)

tsc -p tsconfig.build.json --noEmit is clean. All 46 release-related tests in factory.test.ts pass.

Not touched

No merge, no publish, no tag, no deploy, no change to factory.config.json. The production lease was not cleared and nothing was restarted — that is the principal's call and is being handled separately.

🤖 Generated with Claude Code

Review round 2 (cubic-dev-ai, both threads addressed in 3c02a60)

P1 — the fix had the hole it was fixing. The handback ran after #writeInFlightRegistry(), on the happy path only, so a rejecting registry write skipped it and left the abandoned key renewing its lease forever behind nothing louder than a warn. Same shape as the defect under repair, one level up. It is now in a finally spanning every await in #releaseDeadLetteredSlot, and the cleanup drive is armed before the logger.error call — the one remaining thing between "abandoned" and "lease handed back" that could throw.

P2 — the renewal test was vacuous. The reviewer was correct and the red I originally reported for that test was the 10s leaseIsFree deadline, not the renewal property. renewDispatchLifecycle is owner+epoch fenced, so the old assertion held either way, and 200ms never reached the 60s renewal interval. The renewer now runs for real via a test-only dispatchLifecycleRenewMs port.

The sharper point that surfaced while fixing it: renewDispatchLifecycle fences on owner and epoch but not on expiry. A relinquished lease keeps its owner and epoch, so its own former owner can fully resurrect it. Relinquishing the durable lease while leaving the epoch cached buys nothing.

Ablation matrix

Every test is now pinned by ablation rather than by assertion:

And, in the same file against the same bug (epoch retained), the old test passes while the new one fails — vacuity demonstrated, not asserted.

Final green:

 ✓ frees the lease once the release budget is exhausted, so another publisher can claim the key 269ms
 ✓ leaves nothing for the renewal interval to re-stamp on the abandoned key 448ms
 ✓ still hands the lease back when the in-flight registry write fails during cleanup 169ms
      Tests  3 passed | 652 skipped (655)

tsc -p tsconfig.build.json --noEmit clean; 47 release-related tests in factory.test.ts pass.

Base

No rebase was required: this branch's merge-base is 87fcbf3, which is origin/main's tip. #387 (b464fed) and #389 (87fcbf3) were both already in the base — 87fcbf3 is this branch's parent commit. Re-verified against origin/main after the review round; it had not moved.

Explicitly DEFERRED to follow-up (not in this PR)

Two adjacent defects found during the investigation. Both pre-date this PR, neither is on the lease-retention path it fixes, and folding them in would widen a P0 fix's blast radius on a file another lane is editing with autonomous merge authority. Recording them here so they are not lost:

1. The release budget under-counts. #scheduleDispatchLifecycleRetry returns on #dispatchLifecycleRetryTimers.has(key) before #chargeReleaseAttempt runs, so any release issued while a timer is already armed is free. The 1 Hz held-agent-deadline sweep issues exactly such releases. Fixing it means moving the charge ahead of the dedupe guard, which changes charging semantics for capacity and ownership waits too — that needs its own reasoning and its own tests, not a rider on this one.

2. The held-agent-deadline sweep ignores the dead-letter. #sweepHeldAgentDeadlines gates on #abandonedDispatchReasons, not #dispatchLifecycleReleaseAbandoned, and calls #finishDurableRelease directly for a releasing row. It can therefore keep issuing releases against a dead-lettered unit, which plausibly explains the all-day repeating release-failure log even after the bound fired.

On whether (2) means the production symptom persists after this PR: the blocking symptom does not, the log noise may. #releaseDeadLetteredSlot calls batch.complete(record.issue), which removes the record from inFlight, and the sweep only iterates inFlight — so the sweep stops touching it in this process unless the record is restored from the registry. The four-issues-blocked-on-a-lease symptom is fixed regardless, because that was caused by lease retention, which is what this PR ends. What this PR converts the failure into is bounded rounds: a successor claims the freed key, gets a fresh budget, spends 10 attempts, dead-letters, and hands the lease back again. That is #379's documented intent ("a takeover or a restart re-drives it from the persisted phase") and it no longer blocks any other key. If the agents never come back, that is a slow hot-potato rather than a permanent block — worth its own issue, and it is what (1) and (2) would tighten.

Review round 3 (cubic-dev-ai, both threads addressed in 1303ceb)

P2 — the handback was still losable to a race, and my own comment claimed otherwise. #renewDispatchLifecycles iterates a snapshot of the owned-epoch map, so dropping the epoch before the durable release only stops a tick that starts afterwards. A tick already in flight still carries the key — and releaseDispatchLifecycleLease relinquishes by dropping leaseUntilMs while leaving owner and epoch in place, which are exactly the credentials an owner+epoch-only check accepts. It restored the lease for a full term and the livelock resumed.

renewDispatchLifecycle now fences on expiry as well, in both stores. That is the load-bearing fix: it makes a relinquished lease unrenewable however the handback and an in-flight renewal interleave. A live re-read of the epoch map inside the renewal loop narrows the window ahead of it but does not close it alone, and the comment now says so rather than overclaiming.

This closes a contract inconsistency rather than adding a rule. saveDispatchLifecycle and promoteDispatchLifecycle already refuse when leaseUntilMs <= nowMs, and so does the discovery sweep renewal (renewDiscoverySweepWithDetails returns reason: 'expired'). Dispatch-lifecycle renewal was the only lease operation in the store that did not check expiry.

P3 — the dispatchLifecycleLeasesLost assertion was justified by reasoning that contradicted the paragraph above it, and detected nothing. In the bug shape the renewal succeeds (owner and epoch both match), so no lease is ever counted lost and the counter stays undefined either way. Removed, with a note recording why, so it does not get re-added. The leaseIsFree sampling is the whole of the detection.

Ablation matrix (updated)

Verification on the final tree:

 ✓ frees the lease once the release budget is exhausted, so another publisher can claim the key 278ms
 ✓ leaves nothing for the renewal interval to re-stamp on the abandoned key 426ms
 ✓ still hands the lease back when the in-flight registry write fails during cleanup 163ms
 ✓ refuses to renew a relinquished or expired dispatch lifecycle lease (FileStateStore) 88ms
 ✓ refuses to renew a relinquished or expired dispatch lifecycle lease (InMemoryStateStore) 3ms

tsc -p tsconfig.build.json --noEmit clean; 47 release-related tests in factory.test.ts pass; 137 tests across src/state, src/dispatch, release-error and release-state pass with the new fence.

The store fence is asserted against both implementations from one script, because a fence that holds in only one of them is not a fence.